Add methods to create ArrayBuffers from byte arrays #30445
Conversation
|
Hi @savv! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
savv
changed the title
react-native-bot
added
the
Type: Enhancement
Base commit: a023ff0 |
Base commit: a023ff0 |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
…thods to return them
|
You are adding new method to the base JSI interface but only implementing it for JSC. It seems like If we merge it, it will break the Hermes JSI implementation. |
ReactCommon/jsi/jsi/jsi.h
Outdated
| @@ -252,6 +252,7 @@ class JSI_EXPORT Runtime { | |||
|
|
|||
| virtual String createStringFromAscii(const char* str, size_t length) = 0; | |||
| virtual String createStringFromUtf8(const uint8_t* utf8, size_t length) = 0; | |||
| virtual Object createArrayBufferFromBytes(const void* bytes, size_t length) = 0; | |||
There was a problem hiding this comment.
Nit: this declaration probably shouldn't be in the middle of the string-related methods.
There was a problem hiding this comment.
I moved it a bit further below. Does that work?
|
I believe everything under |
ReactCommon/jsi/jsi/jsi.h
Outdated
| @@ -252,6 +252,7 @@ class JSI_EXPORT Runtime { | |||
|
|
|||
| virtual String createStringFromAscii(const char* str, size_t length) = 0; | |||
| virtual String createStringFromUtf8(const uint8_t* utf8, size_t length) = 0; | |||
| virtual Object createArrayBufferFromBytes(const void* bytes, size_t length) = 0; | |||
There was a problem hiding this comment.
As @tmikov mentioned, there are multiple implementations of JSI for different JS engines. In addition to JSC and Hermes, there is also V8, Chakra, and a new ABI safe JSI wrapper for Windows. Maybe we could provide a default (non-functional) implementation, and allow JS engine specific implementations to opt-in over time?
virtual Object createArrayBufferFromBytes(const void* bytes, size_t length) { throw std::runtime_error{"Not implemented."}; }I don't think there is anything like this in JSI already though, so not sure what the right pattern is for expanding API surface area.
There was a problem hiding this comment.
Hi Ryan, I went ahead and added this. There seems to be precedent for this, which makes me think it's a good suggestion:
react-native/ReactCommon/jsi/JSCRuntime.cpp
Line 942 in 9c32140
I also implemented the same functionality in hermes:
facebook/hermes#419
It would be great to see this change make it in. This would be really helpful when dealing with large blocks of data managed by native code but accessible by JavaScript. We would definitely use it in BabylonReactNative. |
|
As per the discussion on facebook/hermes#419 this can be done by getting the ArrayBuffer constructor: https://gist.github.com/mhorowitz/4915d43b8d82ae0eb98a157e689ba23e So I'm closing this PR. |
|
@savv this does still copy the "byte array". In my case I receive a buffer ( |
|
Capturing our discussion for posterity: |
Summary
This PR adds a factory method to the ArrayBuffer class, so that JSI methods can return them as results.
Changelog
[General] [Added] - Factory method for ArrayBuffer in the JavaScriptCore implementation of JSI
Test Plan
I have a test app where I can directly return data from LevelDB and see ArrayBuffers on the JS side! Let me know if I should add a test case to rn-tester.
This change is